Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Logging module to the code. #524

Merged
merged 2 commits into from
Mar 18, 2024
Merged

Add Logging module to the code. #524

merged 2 commits into from
Mar 18, 2024

Conversation

DinoBektesevic
Copy link
Member

Following some discussion the issue that was the cause for reverting the original logging module merge seems to be environment inconsistency. Cuda can build packages with a different standard library implementation than the system default. Merging of logging seems to have updated the env packages, or included something not previously required, which resulted in an ABI incompatibility.

There are many instances of discussions around the internet as how to properly handle this issue (see for example pybind/pybind11#3623), but it seems the issue is that ultimately C/C++ have no way of propagating, validating or enforcing compiler requirements across packages resulting in the fact that one can build two packages in the same environment, that link against 2 different standard library implementations. The culprit seems to have been somewhere in SciPy.

Consequently, I've removed the reverted logging PR and removed the SciPy patch from #506. I'm unable to reproduce the error, so merging of this PR should wait until the environment is fixed.

Copy link
Contributor

@jeremykubica jeremykubica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good. One discussion point about an import that is needed to make it build on my config.

except ImportError:
warnings.warn("Unable to determine the package version. " "This is likely a broken installation.")

# This is needed for compatibility with some older compilers: erfinv needs to be
# imported before other packages though I'm not sure why.
from scipy.special import erfinv
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this line is removed, the tests no longer run on Baldur for me. I get the error:

  File "/astro/users/jkubica/kbmod_logs/kbmod/src/kbmod/__init__.py", line 14, in <module>
    from kbmod.search import Logging
ImportError: /astro/users/jkubica/kbmod_logs/kbmod/src/kbmod/search.cpython-312-x86_64-linux-gnu.so: undefined symbol: _ZNSt15__exception_ptr13exception_ptr10_M_releaseEv

But as soon as I put it back in all the tests pass fine. I have no idea why or what is happening . But the PR works fine with the include.

Copy link
Collaborator

@wilsonbb wilsonbb Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the PR is merged, I'm seeing the same error on baldur. Though no need for a rollback since

from scipy.special import erfinv in __init__.py does seem to fix it for me as well.

Wondering however if a better solution for the environment issue was found?

src/kbmod/fake_data/fake_data_creator.py Show resolved Hide resolved
src/kbmod/search/logging.h Outdated Show resolved Hide resolved
Fix typo

Co-authored-by: Jeremy Kubica <[email protected]>
Copy link
Contributor

@jeremykubica jeremykubica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can go ahead and merge this. The error I'm seeing is something to do with my environment. As long as this works for everyone else, I can figure out the environment problems in parallel.

@DinoBektesevic
Copy link
Member Author

This is not a blocker to anyone, nor add a critical functionality. So I would not be for the idea that blocks one dev from continuing to contribute. I'm still available for a debugging session if you want.

@DinoBektesevic
Copy link
Member Author

Ok, we sorted this out so I'm merging.

@DinoBektesevic DinoBektesevic merged commit 4fec2d1 into main Mar 18, 2024
2 checks passed
@jeremykubica jeremykubica mentioned this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants